test: migrate stats/base/dists/hypergeometric/variance to ULP-based assertions - #14370
Open
kgryte wants to merge 1 commit into
Open
test: migrate stats/base/dists/hypergeometric/variance to ULP-based assertions#14370kgryte wants to merge 1 commit into
stats/base/dists/hypergeometric/variance to ULP-based assertions#14370kgryte wants to merge 1 commit into
Conversation
… assertions Replace computed relative-tolerance comparisons with `isAlmostSameValue` ULP-based assertions in the fixture loops. The JavaScript and C implementations evaluate the same variance formula using different associations, so they require different bounds. The JavaScript implementation computes `n*(K/N)*((N-K)/N)*((N-n)/(N-1))`, whereas the C implementation computes `n*p*(1-p)*((N-n)/(N-1))`, the latter matching the association used by the Julia reference implementation which generated the fixtures. Bounds are the measured minima over the full fixture set: 29 ULP for test.js and 1 ULP for test.native.js. One lower fails in each case. Ref: #11352
stdlib-bot
added
Good First PR
Contributor
Coverage Report
The above coverage report was generated for the changes in this PR. |
kgryte
marked this pull request as ready for review
August 18, 2026 10:58
kgryte
added
Needs Changes
Member
Author
|
We should investigate the divergence between JS and C and determine whether we should align the implementations. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request:
stats/base/dists/hypergeometric/variancefrom computed relative-tolerance comparisons (delta/tolagainst18.0 * EPS * abs( expected[ i ] )) to ULP-based assertions using@stdlib/assert/is-almost-same-value, per [RFC]: Migratemath/base/specialpackages from relative tolerance testing to ULP difference testing (tracking issue) #11352.@stdlib/math/base/special/absand@stdlib/constants/float64/epsrequires from both test files.Only test files are changed; no implementation, fixture, or documentation changes are included.
ULP bounds
The JavaScript and C implementations evaluate the same variance formula using different associations, so they need different bounds:
test/test.jsn * ( K/N ) * ( (N-K)/N ) * ( (N-n)/(N-1) )29test/test.native.jsn * p * ( 1.0-p ) * ( (N-n)/(N-1) )1The C association matches the one used by the Julia reference implementation (
Distributions.var( Hypergeometric( K, N-K, n ) )) that generatedtest/fixtures/julia/data.json, which is why the native bound is much tighter than the JavaScript one.Both values are the measured minima over the full 100-point fixture set:
test/test.jspasses at29and fails at28(worst case:N = 60,K = 59,n = 32).test/test.native.jspasses at1and fails at0.Each suite was run twice at its final bound with identical results. The native add-on was built locally (
node-gyp configure build) so thattest.native.jsactually executed rather than being skipped; its 108 assertions pass. The native results were additionally checked to be unchanged under FMA contraction (-march=native), so the1ULP bound is not sensitive to compiler contraction on this expression.Reviewers may wish to note that the
29ULP figure reflects the JavaScript implementation's association rather than a defect in the tests; tightening it would require changinglib/main.js, which is out of scope for this PR.Related Issues
This pull request has the following related issues:
math/base/specialpackages from relative tolerance testing to ULP difference testing (tracking issue) #11352Questions
29ULP bound fortest.jsacceptable, or would you prefer the JavaScript implementation be re-associated to match the C implementation (and the Julia reference) in a separate PR so that both bounds can be tightened to1?Other
The pre-commit static analysis hook could not run in this environment because it attempts to download the
editorconfig-checkerbinary from GitHub releases, which was not reachable. EditorConfig compliance (LF endings, tab indentation, no trailing whitespace, final newline) andeslint -c etc/eslint/.eslintrc.tests.jswere both verified manually instead; ESLint reports no problems for either file.Checklist
AI Assistance
If you answered "yes" above, how did you use AI assistance?
Disclosure
This PR was written primarily by Claude Code, running as an unattended scheduled task. It selected the package, studied previously converted packages to match the established idiom, performed the conversion, and determined the minimum passing ULP bounds empirically by bisecting on the full fixture set.
@stdlib-js/reviewers
Generated by Claude Code